Skip to content

Test v3 migration#1013

Merged
thunderbiscuit merged 3 commits into
bitcoindevkit:masterfrom
thunderbiscuit:test/v3-migration
Jun 9, 2026
Merged

Test v3 migration#1013
thunderbiscuit merged 3 commits into
bitcoindevkit:masterfrom
thunderbiscuit:test/v3-migration

Conversation

@thunderbiscuit

@thunderbiscuit thunderbiscuit commented May 27, 2026

Copy link
Copy Markdown
Member

This PR adds old databases (v0.32, v1, v2) to the Android assets, and tests the migration paths for each of those into a v3 wallet.

Notes

You will notice that there is a bit of redundancy/crossover between the new DatabaseVersionCompatTest and the current PersistenceTest. As is, the PersistenceTest is really using v2 databases, and so I didn't want to mess with this and combine the two ideas, even though eventually I think we should clean this up. The tests in PersistenceTest are not wrong, because we know (from the new tests in DatabaseVersionCompatTest) that the v2 to v3 conversion is clean and seamless, and so it's all good. The only think I'd like to fix is that they don't merge the ideas of testing (1) using a v2 db into a v3 wallet, and (2) loading from persistence itself (single and double descriptor wallets, as well as the v0.32 migration).

Checklists

Documentation

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing
  • I've added exactly one changelog:* label
  • I've linked the relevant upstream docs or specs above

@reez reez left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding these migration items and tests!

Your note about the overlap with PersistenceTest makes sense to me, I think keeping DatabaseVersionCompatTest separate is fine for sure.

I left one question on the v0.32 migration test path, plus a couple of non blocking nits.

Nothing else jumped out to me overall this looks good.

persister = newV3DB,
)

wallet.revealAddressesTo(KeychainKind.EXTERNAL, 6u)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this use the preV1Keychains values we just read instead of hardcoding 6u and 0u here? I think that would make the test cover the migration path more directly (read old metadata, apply indices to new v3 wallet, then persist/reload + check that next external/internal addresses are 7 and 1).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes absolutely, that's an oversight for sure (hardcoding the correct answer into my test 😆)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep this covers the hardcoded part. Should we also persist and then reload from newV3DB before checking the next addresses? I think that would make this prove the migrated v3 persistence state too instead of only the live wallet after calling revealAddressesTo

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I applied this with the following:

val wallet = Wallet(
    descriptor = descriptor,
    changeDescriptor = changeDescriptor,
    network = Network.REGTEST,
    persister = newV3DB,
)

wallet.revealAddressesTo(KeychainKind.EXTERNAL, externalPreV1Keychain.lastDerivationIndex)
wallet.revealAddressesTo(KeychainKind.INTERNAL, internalPreV1Keychain.lastDerivationIndex)
wallet.persist(newV3DB)

val reloadedWallet = Wallet.load(
    descriptor = descriptor,
    changeDescriptor = changeDescriptor,
    persister = newV3DB,
)

val addressInfo: AddressInfo = reloadedWallet.revealNextAddress(KeychainKind.EXTERNAL)
val changeAddressInfo: AddressInfo = reloadedWallet.revealNextAddress(KeychainKind.INTERNAL)

assertEquals(addressInfo.index, 7u)
assertEquals(changeAddressInfo.index, 1u)

Comment thread bdk-android/lib/src/androidTest/assets/README.md Outdated
@thunderbiscuit thunderbiscuit force-pushed the test/v3-migration branch 3 times, most recently from 87034ea to c7b6529 Compare May 27, 2026 15:17
@thunderbiscuit thunderbiscuit force-pushed the test/v3-migration branch 2 times, most recently from 594c316 to f702a1d Compare June 4, 2026 18:13
@thunderbiscuit thunderbiscuit requested a review from reez June 4, 2026 18:14
assertEquals("rn0zejch", externalPreV1Keychain.checksum)
assertEquals("j82ry8g0", internalPreV1Keychain.checksum)

val newV3DBFilePath = context.getDatabasePath("new_v3_wallet.sqlite3")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make sure this test starts from a clean new_v3_wallet.sqlite3 database before creating the persister? Wallet(...) creates a new persisted wallet, and upstream returns DataAlreadyExists if the persister already contains wallet data, so repeated runs on the same app data can fail here before testing the migration path.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow good catch. I didn't realize but on local tests reusing an old database already there could definitely be an issue. Fixed now with the following:

// This ensures local tests always create a new DB and don't reuse an old one leftover from prior tests
if (newV3DBFilePath.exists()) newV3DBFilePath.delete()

@thunderbiscuit thunderbiscuit force-pushed the test/v3-migration branch 2 times, most recently from f0c2eb3 to 21f8d55 Compare June 5, 2026 19:01

@reez reez left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 21f8d55

LGTM!

@thunderbiscuit thunderbiscuit merged commit 8f6a10b into bitcoindevkit:master Jun 9, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants